fix(envs): read reward and done from the step envelope in typed clients - #1017
fix(envs): read reward and done from the step envelope in typed clients#1017sergiopaniego wants to merge 1 commit into
Conversation
burtenshaw
added
bug
Cursor
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 77ead67. Configure here.
| reward=data.get("reward"), | ||
| done=data.get("done", False), | ||
| observation = AdvocacyObservation( | ||
| **obs_data, metadata=metadata, reward=reward, done=done |
There was a problem hiding this comment.
Kwargs collide on observation parse
Medium Severity
Building AdvocacyObservation with **obs_data plus explicit reward and done kwargs raises TypeError when those keys are already present in the observation dict. The previous constructor accepted that shape, and test_client_parses_step_result still uses it. That failure is easy to miss because the module is gated by importorskip("sophistry_bench_sprint").
Reviewed by Cursor Bugbot for commit 77ead67. Configure here.
There was a problem hiding this comment.
Alignment Review Report
PR #1017 — fix(envs): read reward and done from the step envelope in typed clients (@sergiopaniego)
Automated Checks
- Lint: PASS — all four changed files pass
ruff format --check,ruff check, andusort check(verified standalone viauvx; the new test lives in CI-scopedtests/). Repo-widelint.shstill exit-1s on ~20 pre-existingenvs/**reformat candidates unrelated to this PR. - Debug code: CLEAN —
check-debug.shhits are all pre-existing insrc/openenv/cli/**; none in the changed files.
What the PR does (verified correct)
serialize_observation (src/openenv/core/env_server/serialization.py:155) intentionally excludes reward/done from the nested observation dict and surfaces them on the envelope. The three clients read them from obs_data instead, so they always saw the defaults:
- chess: both the observation and
StepResult(which is derived fromobservation.reward) were wrong → every step reported reward0.0/ doneFalse. - sumo & sophistry:
StepResultwas already correct, butobservation.reward/donewereNone/False, disagreeing withStepResult.
The fix reads from the envelope for all three, so the observation now agrees with StepResult. Models accept the assignments (base Observation.reward is bool|int|float|None; SumoObservation.reward is Optional[float]). I ran the new tests — the logic is correct.
Open RFCs Context
No conflict. Relevant context: RFC 001 (abstractions — StepResult/Observation) and RFC 002 (env spec + rewards inside environment). This PR only reads server-computed reward/done client-side (no external reward computation), so the rewards-in-environment invariant holds. Client-server separation holds — the three clients import only openenv.core.* + .models, never server/ (the test imports the core serializer, which is fine). No RFC governs whether the typed observation mirrors the envelope's reward/done.
Tier 1: Fixes Required
-
tests/envs/test_client_step_result_contract.py— the new test is not self-contained. It defers everyfrom envs.<env> importinto the test bodies with no module-levelenvs.*import, so run via the repo's own documented single-file command it errors on all 6 withModuleNotFoundError: No module named 'envs'.envsis a namespace package (noenvs/__init__.py) importable only via cwd under barepython; the console-scriptpytestused by CI and.claude/hooks/test.shdoesn't add cwd, so it passes only in the full suite where a sibling pure-Python env test (e.g.test_connect4_env.py) importsenvs.*at module scope during collection and primessys.modules['envs']. See inline for the one-line fix. (CI is green today — 1497 passed in the CI-scope suite — so this is standalone/robustness, not a build breaker.) -
envs/sophistry_bench_sprint_env/models.py:29-34(out of this PR's diff) — theAdvocacyObservationdocstring tells callers to read reward fromStepResult.rewardnotobservation.rewardbecause "onlyStepResult.rewardcarries the weighted aggregate." This PR makes the client populateobservation.reward/done, so that guidance now contradicts the code — update it. (See inline on the sophistry client.)
Tier 2: Alignment Discussion
Principle Conflicts
None — the change is a consistency positive: ~20 other env clients (atari, browsergym, connect4, maze, snake, unity, …) already pass done/reward from the envelope into their observation constructor. This PR brings three stragglers into line.
ALIGNMENT FLAG (scope/consistency, low priority): agent_world_model_env has the same latent disagreement, untouched.
- Principle at stake: "one canonical way" (PRINCIPLES §What We Trade Off) + the observation↔
StepResultreward agreement this PR establishes. - The concern:
envs/agent_world_model_env/client.py:68-75buildsAWMObservation(**obs_data)(obs_data has reward/done excluded by the serializer) and sets reward/done only on theStepResult, soobservation.reward/donestayNone/Falsethere too. Not required for this PR, but a natural follow-up if typed observations are meant to carry reward/done. - Suggested reviewer: Zhaoyang Wang (AWM author); @Darktex for the reward-contract convention.
RFC Conflicts
None identified.
Suggested Reviewers (git blame)
- Reward-on-envelope / observation-reward contract → @Darktex (Davide Testuggine — authored the
exclude={"reward","done"}decision inserialization.py:155; owns rewards inside environment). - Stale
AdvocacyObservationdocstring → Anusha Acharya (authored it). - AWM scope note → Zhaoyang Wang.
Summary
- 2 mechanical items to fix (non-self-contained test; stale docstring) — neither breaks CI.
- 1 alignment/scope point for human review (AWM has the same pattern).
- 0 RFC conflicts.
Sent by Cursor Automation: Pre-review
|
|
||
| from __future__ import annotations | ||
|
|
||
| from openenv.core.env_server.serialization import serialize_observation |
There was a problem hiding this comment.
Tier 1 — test is not self-contained (fails standalone, passes only in the full suite).
All from envs.<env> import statements are deferred into the test bodies, and there's no module-level envs.* import here. Run on its own via the repo's documented single-file command, every test errors:
PYTHONPATH=src:envs uv run pytest tests/envs/test_client_step_result_contract.py
# ModuleNotFoundError: No module named 'envs' (6 failed)
envs is a namespace package (no envs/__init__.py), resolvable only via cwd (sys.path[0]=='') under bare python. The console-script pytest (CI test.yml + .claude/hooks/test.sh) doesn't put cwd on sys.path, so this only passes in the full tests/ run, where a sibling pure-Python env test (e.g. test_connect4_env.py) imports envs.* at module scope during collection and primes sys.modules['envs']. Pairing it with test_chess_environment.py still fails, since that module is importorskip-gated on chess/moonfish and skips before priming.
Fix — import at module scope like every other file in tests/envs/ (also lets you drop the in-body imports):
from envs.chess_env.client import ChessEnv
from envs.chess_env.models import ChessObservation
from envs.sumo_rl_env.client import SumoRLEnv
from envs.sumo_rl_env.models import SumoObservation
from envs.sophistry_bench_sprint_env.client import SophistryBenchSprintEnv
from envs.sophistry_bench_sprint_env.models import AdvocacyObservationThese modules need only openenv+pydantic (no engine), so a top-level import is safe and self-primes envs — consistent with this file's own "no importorskip" rationale.
| reward=data.get("reward"), | ||
| done=data.get("done", False), | ||
| observation = AdvocacyObservation( | ||
| **obs_data, metadata=metadata, reward=reward, done=done |
There was a problem hiding this comment.
Correct — populating reward/done here makes observation.reward agree with StepResult.reward, matching the majority of env clients. **obs_data is safe against a kwarg collision because serialize_observation excludes reward/done from the observation dict.
Follow-up (Tier 2, out-of-diff): the AdvocacyObservation docstring at envs/sophistry_bench_sprint_env/models.py:29-34 still instructs callers to read reward from StepResult.reward not observation.reward because "only StepResult.reward carries the weighted aggregate." That now contradicts this change — please update it to note the typed client mirrors reward/done onto the observation. (author: Anusha Acharya)
| # `serialize_observation` keeps reward and done on the envelope, not in | ||
| # the observation dict. Reading them from obs_data left every step | ||
| # reporting reward 0.0 and done False. | ||
| reward = payload.get("reward", 0.0) |
There was a problem hiding this comment.
Minor consistency nit (non-blocking): this defaults to 0.0, whereas sumo_rl_env and sophistry use payload.get("reward") (default None). Harmless here — serialize_observation always emits the reward key so the default is never hit, and ChessObservation.reward accepts None — but None would be more consistent with the base Observation.reward default and the sibling clients.
serialize_observation() excludes reward and done from the nested observation dict and surfaces them on the response envelope. Three clients read them from the observation dict instead, so they got the defaults. chess_env was the worst case: StepResult.reward was always 0.0 and StepResult.done always False, so a chess training loop never observed a reward or an episode ending. sumo_rl_env and sophistry_bench_sprint_env built a correct StepResult but left observation.reward as None, trapping callers that read result.observation rather than result. Adds tests/envs/test_client_step_result_contract.py, which drives each client's _parse_result with real serializer output and pins the wire contract. The chess cases deliberately do not importorskip on python-chess: neither the client nor the models import it, so skipping would have hidden this exact regression.
77ead67 to
8035807
Compare
Summary
serialize_observation()deliberately excludesrewardanddonefrom the nestedobservation dict and surfaces them on the response envelope. Three typed clients read
them from the observation dict instead, so they silently received the field defaults.
chess_envis the worst case:StepResult.rewardwas always0.0andStepResult.donealwaysFalse, so a chess training loop never observed a reward oran episode ending.
sumo_rl_envandsophistry_bench_sprint_envbuilt a correctStepResultbut leftobservation.rewardasNone, which traps any caller that readsresult.observationrather thanresult.Type of Change
Alignment Checklist
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violatedTest Plan
Adds
tests/envs/test_client_step_result_contract.py, which drives each client's_parse_resultwith realserialize_observation()output rather than a hand-writtenpayload, so it fails if a client and the wire format ever drift apart again. It also pins
the wire contract itself, so a future change to the envelope shape is caught on the
serializer side.
Reproduce the chess bug on
main:Checked out at
origin/mainwith only the new test file applied, 3 of the 6 cases fail,one per affected client. With the fix all 6 pass, and the full suite is green:
1613 passed, 127 skipped.The chess cases deliberately do not
importorskiponpython-chess. Neitherenvs/chess_env/client.pynorenvs/chess_env/models.pyimports it, so skipping when theengine is absent would have quietly skipped the very regression being guarded, which is
how this went unnoticed.
I audited every other env client for the same pattern; the remaining 26 already read from
the envelope.
Claude Code Review
N/A
Note
Medium Risk
Fixes RL/training-critical step semantics that were wrong in production paths; behavior change is intentional but any code that depended on the buggy defaults could see different outcomes.
Overview
Fixes a wire-format mismatch in three env clients:
serialize_observationputsrewardanddoneon the response envelope, not inside the nestedobservationdict. Clients that read those fields fromobservationsilently got defaults instead of real step outcomes.chess_envwas the worst case—every step reportedStepResult.reward == 0.0anddone == False, so training loops never saw rewards or episode end.sumo_rl_envandsophistry_bench_sprint_envbuilt correct top-levelStepResultvalues but leftobservation.reward/donewrong for callers that read the observation object.Each affected
_parse_resultnow takesrewardanddonefrom the envelope and mirrors them on the typed observation soresult.observationagrees withresult.Adds
tests/envs/test_client_step_result_contract.py, which feeds realserialize_observation()output into each client and pins the envelope contract so serializer and clients cannot drift again.Reviewed by Cursor Bugbot for commit 8035807. Bugbot is set up for automated code reviews on this repo. Configure here.